Skip to content

Conversation

@Turalchik
Copy link
Contributor

@Turalchik Turalchik commented Oct 10, 2025

Close #3595.

@Turalchik Turalchik marked this pull request as draft October 10, 2025 09:28
@codecov
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

❌ Patch coverage is 88.38583% with 59 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.49%. Comparing base (3d162dc) to head (8cccd96).
⚠️ Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
pkg/smartcontract/manifest/parameter.go 78.31% 11 Missing and 7 partials ⚠️
pkg/smartcontract/rpcbinding/binding.go 73.43% 16 Missing and 1 partial ⚠️
pkg/smartcontract/manifest/extended_type.go 92.99% 10 Missing and 5 partials ⚠️
pkg/compiler/debug.go 91.86% 6 Missing and 1 partial ⚠️
pkg/compiler/codegen.go 95.83% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4036      +/-   ##
==========================================
+ Coverage   72.48%   74.49%   +2.01%     
==========================================
  Files         364      365       +1     
  Lines       56750    47958    -8792     
==========================================
- Hits        41135    35727    -5408     
+ Misses      13880    10490    -3390     
- Partials     1735     1741       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • semantic checks
  • stack item serialization

See neo-project/neo#4043 also

Overloads map[string]string `yaml:"overloads,omitempty"`
NamedTypes map[string]binding.ExtendedType `yaml:"namedtypes,omitempty"`
Overloads map[string]string `yaml:"overloads,omitempty"`
NamedTypes map[string]manifest.ExtendedType `yaml:"namedtypes,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely it's no longer needed here, manifest has it all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to write an UnmarshalYAML method for ProjectConfig or mark the NamedTypes field as deprecated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just drop them and then let's see how it goes.


import (
"encoding/json"
"gopkg.in/yaml.v3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely we no longer need yaml here.

@Turalchik Turalchik force-pushed the support-nep-25 branch 3 times, most recently from 0457d1a to a8966da Compare October 20, 2025 06:26
@Turalchik Turalchik marked this pull request as ready for review October 20, 2025 06:26
@AnnaShaleva AnnaShaleva changed the title smartcontract: add support NEP-25 smartcontract: add support for NEP-25 Oct 22, 2025
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, adjust the PR/commit description, it points to the wrong issue.

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, check the compatibility with the reference implementation, especially in the part of extended type verification and circular references.

Comment on lines 26 to +33
extendedtype:
base: Struct
name: crazyStruct
fields:
- field: I
base: Integer
- field: B
base: Boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you include fields in the definition of named extended type? It shouldn't be like that, all named types should be specified in the namedtypes section, and extended type here should just point to a specific named type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer store namedtypes in the config, so I don't quite understand where the author should specify the definition of named type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer store namedtypes in the config

They still should be presented in a separate section of config file, like it's done for manifest (https://github.com/neo-project/proposals/blob/master/nep-25.mediawiki#contract). Otherwise, we have to declare named type in-place every time it's used in the contract config. Consider the same named type used in multiple events, for example.

This namedtypes section of config will be relevant for events only and will likely be removed once we solve #3027. Until this issue is solved, namedtypes section is the only place that can be used for manifest's events construction.

@roman-khimov, I need your opinion here because it was your request (https://github.com/nspcc-dev/neo-go/pull/4036/files#r2419192068). So far, we used config-based events for type casting at the compiler level and to compare the emitted event types with the desired ones. If we still want to support this feature, then we need namedtypes section to be resented in the contract config. ACK?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, keeping them isn't hard. I've not seen code throwing structures in events (other than examples), but #3027 seems to be the key here.

if err := node.Decode(&raw); err != nil {
return err
}
if v, ok := raw["base"]; ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, there shouldn't be base field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON format should be compatible with C#.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I needed to support the old format, but I don't need to do that, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. No old format support, we'll migrate to the NEP-25 without transition period.

if e.Base != smartcontract.InteropInterfaceType {
return fmt.Errorf("`ExtendedType.Interface` field can not be specified for %s", e.Base)
}
if e.Interface != "IIterator" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a constant for IIterator in our codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the constant only here

const iteratorInterfaceName = "IIterator"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it exported, move to a suitable package and reuse.

}
e.Fields[i] = p
}
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should heck if the resulting type (and all nested ones) are valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we call the Valid() method? why don't we do this for, for example, manifest.Parameter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we call the Valid() method?

Yes. Check the reference implementation for that.

why don't we do this for, for example, manifest.Parameter?

Check how it's handled in the reference implementation and attach the link to the discussion.

Close #3595.

Signed-off-by: Tural Devrishev <[email protected]>
@AnnaShaleva AnnaShaleva added the blocked Can't be done because of something label Nov 6, 2025
Comment on lines 26 to +33
extendedtype:
base: Struct
name: crazyStruct
fields:
- field: I
base: Integer
- field: B
base: Boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer store namedtypes in the config

They still should be presented in a separate section of config file, like it's done for manifest (https://github.com/neo-project/proposals/blob/master/nep-25.mediawiki#contract). Otherwise, we have to declare named type in-place every time it's used in the contract config. Consider the same named type used in multiple events, for example.

This namedtypes section of config will be relevant for events only and will likely be removed once we solve #3027. Until this issue is solved, namedtypes section is the only place that can be used for manifest's events construction.

@roman-khimov, I need your opinion here because it was your request (https://github.com/nspcc-dev/neo-go/pull/4036/files#r2419192068). So far, we used config-based events for type casting at the compiler level and to compare the emitted event types with the desired ones. If we still want to support this feature, then we need namedtypes section to be resented in the contract config. ACK?

Comment on lines +982 to +988
name := "unnamed"
if c.buildInfo.options.CollectedNamedTypes != nil {
for c.buildInfo.options.CollectedNamedTypes[name].Name == name {
name = name + "X"
}
_ = c.genStructExtended(typ, name, c.buildInfo.options.CollectedNamedTypes)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicating code.

return
}
var (
seen = make(map[types.Type]bool)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map[types.Type]struct{}

// GuessEventTypes specifies if types of runtime notifications need to be guessed
// from the usage context. These types are used for RPC binding generation only and
// GuessedNamedTypes specifies guessed from the usage context named types of
// runtime notifications. These types are used for RPC binding generation only and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a note that it's nil in case if ...

// can be defined for events with name known at the compilation time and without
// variadic args usages. If some type is specified via config file, then the config's
// one is preferable. Currently, event's parameter type is defined from the first
// variadic args usages. Currently, event's parameter type is defined from the first
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes regarding config file may be reverted after the resolution of https://github.com/nspcc-dev/neo-go/pull/4036/files#r2511308314.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just don't forget we have two configs currently, contract-level and bindings-level, contract one stays in some form anyway, but bindings only need a manifest.

Comment on lines +52 to +53
pa := &e.Fields[i]
pb := &other.Fields[i]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a and b.

if err := node.Decode(&raw); err != nil {
return err
}
if v, ok := raw["base"]; ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. No old format support, we'll migrate to the NEP-25 without transition period.

}
e.Fields[i] = p
}
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we call the Valid() method?

Yes. Check the reference implementation for that.

why don't we do this for, for example, manifest.Parameter?

Check how it's handled in the reference implementation and attach the link to the discussion.

Comment on lines +262 to +264
if !unicode.IsLetter(rune(e.Name[0])) {
return errors.New("`ExtendedType.Name` must start with a letter")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use regexp, then this logic should be a part of regexp. Check the reference implementation.

if e.Base != smartcontract.InteropInterfaceType {
return fmt.Errorf("`ExtendedType.Interface` field can not be specified for %s", e.Base)
}
if e.Interface != "IIterator" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it exported, move to a suitable package and reuse.

@AnnaShaleva
Copy link
Member

alphaNumDot = regexp.MustCompile(`^[A-Za-z0-9.]+$`)
)

type ExtendedType struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exported type needs comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked Can't be done because of something

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support extended SC types

4 participants